Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

General stuff #298

Closed

Conversation

MissAllSunday
Copy link
Contributor

Just some general stuff:

  • Fixed some typos.
  • Added a description to loadGeneralSettingParameters().
  • Add a function to load custom profile fields without the need to use loadMemberContext() it adds the values to $memberContext but it was designed to be used as a stand alone function to use when you only need to use the profile fields.

Signed-off-by: Suki <suki@missallsunday.com>
Signed-off-by: Suki <suki@missallsunday.com>
without having to use loadMemberContext()

Signed-off-by: Suki <suki@missallsunday.com>
Signed-off-by: Suki <suki@missallsunday.com>
…st...

Signed-off-by: Suki <suki@missallsunday.com>
@MissAllSunday
Copy link
Contributor Author

OK, for the loadMemberCustomFields($users) function makes any sense it gotta have an extra param, a title param to get specific rows and not the entire themes table... or better yet, an array or titles to be able to get multiple profile fields at once.

I'll get to that later, kudos goes to Oldiesmann for letting me know about this on IRC.

Signed-off-by: Suki <suki@missallsunday.com>
@MissAllSunday
Copy link
Contributor Author

OK MissAllSunday@e2bf6e0 sets a better and cleaner version. It nows pass all possible variables for each field and supports multiple field names as well as multiple users.

Signed-off-by: Suki <suki@missallsunday.com>
FROM {db_prefix}themes AS t
LEFT JOIN {db_prefix}custom_fields AS c ON (c.col_name = t.variable)
WHERE id_member' . (count($users) == 1 ? ' = {int:loaded_ids} ' : ' IN ({array_int:loaded_ids}) ') .
'AND variable' . (count($params) == 1 ? ' = {string:params}' : ' IN ({array_string:params})'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not use just IN?
That would save you some comparisons:

    WHERE id_member IN ({array_int:loaded_ids})
      AND variable IN ({array_string:params})',

and here below:

      'loaded_ids' => $users,
      'params' => $params,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because thats how SMF do things... I just followed and copy/paste SMF coding style.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay...never checked before... 👼

/me feels is a bit odd
I'm not sure if there is any practical difference...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There isn't if you aren't picky in terms of performance, it is if you want the code to be more organized but there is a bazillion other places where SMF does this, might as well go with the flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SMF avoids using 'IN' in the query, for the query to use the index instead. There's only an index on id_member iirc however.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my (quick) test with EXPLAIN, MySQL is smart enough to understand there is just one value and uses the index anyway. Of course if there is more than one then it doesn't.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks for checking it out!

@MissAllSunday MissAllSunday deleted the general_stuff branch February 21, 2013 18:21
@emanuele45 emanuele45 mentioned this pull request Feb 24, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants